Repository navigation
[CI Visibility] Fix message loss and abandoned mutexes in the IPC channel - #9335
Merged
Merged
Conversation
tonyredondo
force-pushed
the
tony/ci-visibility-ipc-cross-runtime-lock
branch
from
September 30, 2026 11:55
6ff2985 to
8fc4f00
Compare
tonyredondo
marked this pull request as ready for review
September 30, 2026 12:07
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
BenchmarksBenchmark execution time: 2026-10-27 14:06:36 Comparing candidate commit ea54312 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 71 metrics, 1 unstable metrics, 5 known flaky benchmarks, 121 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9335) and master. ✅ No regressions detected |
anmarchenko
approved these changes
Sep 30, 2026
gnufede
approved these changes
Sep 30, 2026
tonyredondo
force-pushed
the
tony/ci-visibility-ipc-cross-runtime-lock
branch
from
September 30, 2026 16:28
8fc4f00 to
ebb8189
Compare
tonyredondo
force-pushed
the
tony/ci-visibility-ipc-cross-runtime-lock
branch
from
October 23, 2026 10:42
ebb8189 to
1e14acd
Compare
Member
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e14acdcb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tonyredondo
force-pushed
the
tony/ci-visibility-ipc-cross-runtime-lock
branch
from
October 23, 2026 12:05
1e14acd to
bf0693b
Compare
tonyredondo
changed the title
Opening a CircularChannel reset its read and write pointers, so every new IPC client dropped the messages the other side hadn't read yet. Only a new file, or one with an unexpected size, is initialized now.
A completely full buffer was marked with a virtual write position (BufferBodySize + writePos), which overflows the ushort pointer with the default 64 KB buffer. The reader then looped over the same messages forever while holding the lock, until the process ran out of memory. Always keep one byte free instead, so the write position never reaches the read position.
tonyredondo
force-pushed
the
tony/ci-visibility-ipc-cross-runtime-lock
branch
from
October 23, 2026 13:38
bf0693b to
a84c18a
Compare
…or writing WaitOne grants ownership of an abandoned mutex before it throws AbandonedMutexException. The CircularChannel reader already handled that, but the constructor and the writer let the exception escape, so the call failed and the thread kept owning the mutex, which blocked every other process using the channel. All three paths now share the reader's handling. This resolves the TODO in CheckBuildLogsForErrors, so the ignore patterns for those errors are removed. The pattern for ReleaseMutex failing after a successful wait stays: that one comes from the .NET 11 RC named mutex bug (dotnet/runtime#134491) and goes away with .NET 11 GA.
The reader slept between polls and returned early once the reader or the channel was disposed, so anything written during its last polling interval was dropped. A session that disposes its IPC server right after the last message from a child process could lose that message. The reader now waits on the stop event instead of sleeping, so Dispose wakes it right away, and it polls once more before it stops. Dispose already joins the polling thread before the channel releases the memory mapped file.
tonyredondo
force-pushed
the
tony/ci-visibility-ipc-cross-runtime-lock
branch
from
October 23, 2026 15:16
a84c18a to
94750e5
Compare
andrewlock
approved these changes
Oct 27, 2026
tonyredondo
added a commit
that referenced
this pull request
Oct 29, 2026
…ped (#9486) ## Summary of changes When VSTest runs no tests and the run succeeds, the test session is now reported as skipped: - `test.status:skip` - `test.skip_reason:No tests were executed` - `test.session.empty_reason:zero_tests` This is what you get from a filter that matches nothing, with `dotnet test`, `dotnet vstest` or `vstest.console`. Sessions that ran tests, and failed sessions, are reported as before. The tag, value and skip reason are the same as in the Go version (DataDog/dd-trace-go#5697) and datadog-ci-rb; dd-trace-js uses the same tag and value. This is an alternative to #9325. It relies on the IPC channel fixes from #9335. ## Reason for change An empty run that exits successfully was reported as `pass`, so it looked the same as a session whose test events were lost. ## Implementation details - A new CallTarget integration hooks vstest.console's `RunTestsArgumentExecutor+TestRunRequestEventsRegistrar.TestRunRequest_OnRunCompletion`, the handler VSTest uses to print "No test matches the given testcase filter". Its signature hasn't changed from vstest 15.9 to today, and it only runs for command-line runs, so IDE (design mode) runs aren't affected. The count comes from VSTest itself, which means it includes tests from frameworks we don't instrument, and a session without test events can't be mistaken for an empty one. An aborted, canceled or failed run, or one without statistics, counts as unknown. - vstest.console sends the count to the session it inherited the context from, as a new `SessionExecutedTestsMessage` on the session's existing IPC channel. That session lives in vstest.console itself (`dotnet vstest`, `vstest.console`, and `dotnet test` on SDK 10 and later) or in the `dotnet` CLI process (`dotnet test` on SDK 9 and earlier). Only sessions created by the dotnet test integrations host the IPC server, so manual sessions never get a count. - `TestSession` keeps one small state: nothing reported, every report was zero, or a runner ran tests or couldn't count them. A session that closes as passed with only zero reports closes as skipped. The check runs after the IPC server is disposed, so every report already sent has been read. A runner exception counts as unknown, since CallTarget passes exit code 0 in that case. - The count travels over the session's IPC channel, so this needs the `CircularChannel` fixes in #9335: a new client no longer drops the session's unread messages, the reader reads what was written before it's disposed, and an abandoned mutex no longer leaves the channel locked. - Test modules that report to a session in another process also send how many tests they created when they close, with the same message. VSTest's count covers frameworks we don't instrument, and the module counts cover runners that don't report one, such as Microsoft.Testing.Platform under `dotnet test` on SDK 9 and earlier, where MTP projects share the CLI's session with VSTest ones. Both senders use `TestSession.TrySendExecutedTests`, which retries a full channel for up to a second. - The hook finds the session with the same lookup the Coverlet and Microsoft CodeCoverage integrations use. Each of them had a private copy, and `DotnetCommon` repeated it inline twice, so it now lives once in `DotnetCommon.TryGetParentSessionSpanId`. Compared with #9325: - #9325 only recorded the count when the session was in the same process as vstest.console, so `dotnet test` on SDK 9 and earlier never got the new status. - #9325 adds two hooks on `Microsoft.VisualStudio.TestPlatform.Client`, a new target assembly that needs Honeypot and trimming entries, plus per-request tracking with `ConditionalWeakTable`s. This PR has one hook in vstest.console, which we already instrument. ## Test coverage - `EmptyTestSessionTests`: zero, non-zero, missing, aborted, canceled and failed runs, going through the real IPC path; zero reports from every runner; a test module that ran tests keeps a session that VSTest reported as empty passing, and one that ran none doesn't; a failed session; a runner exception; a session without an IPC server. It mocks `ITestOptimization` to skip the 5 s agent discovery, so each test takes milliseconds. - `VSTestEmptySessionTests` (integration, existing xUnit sample, real vstest.console): a filter that matches nothing gives a skipped session, and a matching test gives a passed one. With a parent session injected through environment variables, vstest.console sends 0 or 1 to it; those cases disable the xUnit integration, so the testhost's module doesn't send its own count. Both of those cases fail if the hook doesn't send the count. These tests skip for net8.0 to net10.0 on Linux and macOS while CI uses the .NET 11 RC SDK, because .NET 11 RC can't share named mutexes with .NET 8-10 (dotnet/runtime#134491). The SDK check that `MsTestV2DiscoveryTests` already had now lives in `TestingFrameworkEvpTest` so both classes share it. vstest.console runs on the SDK, so the other target frameworks still cover the same code. - Manual `dotnet test` runs with the profiler attached: on SDK 9 the `dotnet` CLI session received the count and closed as skipped (0) or passed (1), and on SDK 10 the vstest.console session did the same. - Local runs on Linux with net8.0: the whole `Datadog.Trace.Tests.Ci` namespace, the new integration tests, and the Release build of `Datadog.Trace` with no warnings. ## Other details - Known gaps: - If a solution mixes VSTest and Microsoft.Testing.Platform projects under `dotnet test` on SDK 9 or earlier, MTP projects that use a framework we don't instrument don't send a count. If the VSTest projects run no tests, the session can still be reported as skipped. - A count is lost if the session's IPC channel stays full for more than a second, and then another project's zero count could mark the session as skipped. I didn't add a persisted fallback for that; the sender retries a full channel for up to a second. - `/Tests:` runs (`RunSpecificTestsArgumentExecutor`) aren't hooked and stay passed. - Not run locally: Windows, .NET Framework (vstest.console.exe) and macOS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Fixes four bugs in the CI Visibility IPC channel (
CircularChannel) that lose messages or leave the channel locked:ushortpointer with the default 64 KB buffer. The reader then re-read the same messages forever while holding the lock, until the process ran out of memory.AbandonedMutexExceptionescape. An abandoned wait still transfers ownership, so the thread kept the mutex and every other process using the channel timed out. The reader already handled this ([Test Optimization] Fix MSTest 4.4.0 discovery and native retries #9192).Reason for change
Coverage results, session tags and the executed test count in #9486 all go through this channel, and each of these bugs can lose them. The abandoned-mutex one is the TODO that #9317 left in
CheckBuildLogsForErrors.Implementation details
TryAcquireMutex, which treats an abandoned mutex as acquired and logs a warning. Callers release it as usual. TheCheckBuildLogsForErrorsTODO and its ignore patterns for these errors are removed. The pattern forReleaseMutexfailing after a successful wait stays, with a new comment: that one comes from the .NET 11 RC runtime bug below, and it shows up on Linux ARM64 when a .NET 8-10 test host shares a channel with the .NET 11 VSTest runner.This PR doesn't fix the .NET 11 RC named mutex incompatibility with .NET 8-10 on Unix (dotnet/runtime#134491). That's fixed in the runtime for .NET 11 GA (dotnet/runtime#134595), so the existing .NET 11 test skips stay until we move to GA. An earlier version of this PR replaced the named mutex with a file lock on Unix; I dropped it so we don't maintain a separate lock for a runtime bug that's going away.
Test coverage
New
CircularChannelTests, each checked locally to fail without its fix:OpeningAnExistingChannelKeepsUnreadMessagesBufferNeverFillsCompletelyWithTheDefaultSize: with the default buffer size, a message that would fill the buffer exactly is rejected, and every message is delivered exactly once. The unit tests used an 8 KB buffer, which never overflows.OpeningAChannelAndWritingAcquireAnAbandonedMutexDisposingAReaderReadsMessagesWrittenBeforeItCircularChannelWriteTestnow expects one byte to stay free.Datadog.Trace.Tests.Ci.Ipcpasses locally on Linux with net8.0.Other details
Related: #9317, #9327, dotnet/runtime#134491, dotnet/runtime#134595. #9486 is stacked on this PR.